-
Notifications
You must be signed in to change notification settings - Fork 36
[Peras 4] Add ObjectDiffusion and PerasCert diffusion (instance of ObjectDiffusion)
#1679
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
PerasCert diffusion (instace of ObjectDiffusion)PerasCert diffusion (instance of ObjectDiffusion)
PerasCert diffusion (instance of ObjectDiffusion)PerasCert diffusion (instance of ObjectDiffusion)
80d3c69 to
f88b3ae
Compare
dca0315 to
d7c3c64
Compare
37a9f85 to
222f4be
Compare
d7c3c64 to
f1158c0
Compare
222f4be to
d47c331
Compare
PerasCert diffusion (instance of ObjectDiffusion)PerasCert diffusion (instance of ObjectDiffusion)
d47c331 to
918307c
Compare
f1158c0 to
90131b2
Compare
8aba6fc to
8b987f6
Compare
328b50a to
1018f6e
Compare
f8083cc to
8a88eb7
Compare
...onsensus/src/ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ObjectDiffusion/Inbound.hs
Outdated
Show resolved
Hide resolved
...onsensus/src/ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ObjectDiffusion/Inbound.hs
Outdated
Show resolved
Hide resolved
...onsensus/src/ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ObjectDiffusion/Inbound.hs
Show resolved
Hide resolved
agustinmista
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, minor comments only.
...sensus-diffusion/src/ouroboros-consensus-diffusion/Ouroboros/Consensus/Network/NodeToNode.hs
Show resolved
Hide resolved
ouroboros-consensus/changelog.d/20250918_104810_thomas.bagrel_object_diffusion.md
Show resolved
Hide resolved
...s/src/ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ObjectDiffusion/ObjectPool/API.hs
Outdated
Show resolved
Hide resolved
...s/src/ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ObjectDiffusion/ObjectPool/API.hs
Outdated
Show resolved
Hide resolved
...ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ObjectDiffusion/ObjectPool/PerasCert.hs
Outdated
Show resolved
Hide resolved
...consensus/test/consensus-test/Test/Consensus/MiniProtocol/ObjectDiffusion/PerasCert/Smoke.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/test/consensus-test/Test/Consensus/MiniProtocol/ObjectDiffusion/Smoke.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/test/consensus-test/Test/Consensus/MiniProtocol/ObjectDiffusion/Smoke.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/test/consensus-test/Test/Consensus/MiniProtocol/ObjectDiffusion/Smoke.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/test/storage-test/Test/Ouroboros/Storage/PerasCertDB/StateMachine.hs
Outdated
Show resolved
Hide resolved
tbagrel1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...sensus-diffusion/src/ouroboros-consensus-diffusion/Ouroboros/Consensus/Network/NodeToNode.hs
Show resolved
Hide resolved
...onsensus/src/ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ObjectDiffusion/Inbound.hs
Outdated
Show resolved
Hide resolved
...onsensus/src/ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ObjectDiffusion/Inbound.hs
Show resolved
Hide resolved
...s/src/ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ObjectDiffusion/ObjectPool/API.hs
Outdated
Show resolved
Hide resolved
...ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ObjectDiffusion/ObjectPool/PerasCert.hs
Outdated
Show resolved
Hide resolved
...consensus/test/consensus-test/Test/Consensus/MiniProtocol/ObjectDiffusion/PerasCert/Smoke.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/test/consensus-test/Test/Consensus/MiniProtocol/ObjectDiffusion/Smoke.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/test/consensus-test/Test/Consensus/MiniProtocol/ObjectDiffusion/Smoke.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/test/consensus-test/Test/Consensus/MiniProtocol/ObjectDiffusion/Smoke.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/test/storage-test/Test/Ouroboros/Storage/PerasCertDB/StateMachine.hs
Outdated
Show resolved
Hide resolved
|
During last Wednesday’s meeting, we agreed that it would be better to associate each The idea would be to introduce a class: class
( Ord (IdOf a)
, Eq (IdOf a)
, Show (IdOf a)
, NoThunks (IdOf a)
, Serialise (IdOf a)
) => HasId a where
type IdOf a
getId :: a -> IdOf aand then provide instances such as NOTE: An alternative would be to use a multi-parameter type class with a functional dependency, The current concerns are:
instance (Show objectId, Typeable object, Typeable objectId)
=> Exception (ObjectDiffusionInboundError objectId object)would become: instance (Show (IdOf object), Typeable object, Typeable (IdOf object))
=> Exception (ObjectDiffusionInboundError object)and this seems to require those extensions. Consequently, we’d like to get approval again from the IOG team before going down this route. |
|
I changed the interface of |
agustinmista
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a look at the fixups, and wrote some comments and questions, nothing too serious but some things we should address before merging.
...ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ObjectDiffusion/ObjectPool/PerasCert.hs
Show resolved
Hide resolved
...ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ObjectDiffusion/ObjectPool/PerasCert.hs
Outdated
Show resolved
Hide resolved
...nsensus/src/ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ObjectDiffusion/Outbound.hs
Outdated
Show resolved
Hide resolved
...nsensus/src/ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ObjectDiffusion/Outbound.hs
Outdated
Show resolved
Hide resolved
...nsensus/src/ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ObjectDiffusion/Outbound.hs
Show resolved
Hide resolved
ouroboros-consensus/test/consensus-test/Test/Consensus/MiniProtocol/ObjectDiffusion/Smoke.hs
Outdated
Show resolved
Hide resolved
e0cdbb5 to
a02433c
Compare
|
Ok, now the last small issue on smoke tests has been fixed, so this is ready :) |
...s/src/ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/ObjectDiffusion/ObjectPool/API.hs
Outdated
Show resolved
Hide resolved
Also use defaultMiniProtocolParameters instead of hardcoded value in unstable-diffusion-testlib to account for newly defined parameters in the new `ouroboros-network` version. Also integrate `NodeToNodeV_16`. Co-authored-by: Agustin Mista <[email protected]> Co-authored-by: Alexander Esgen <[email protected]> Co-authored-by: Georgy Lukyanov <[email protected]> Co-authored-by: Thomas BAGREL <[email protected]> Co-authored-by: Nicolas BACQUEY <[email protected]> Co-authored-by: Nicolas "Niols" Jeannerod <[email protected]>
…eader,Writer}` API Co-authored-by: Agustin Mista <[email protected]> Co-authored-by: Alexander Esgen <[email protected]> Co-authored-by: Georgy Lukyanov <[email protected]> Co-authored-by: Thomas BAGREL <[email protected]> Co-authored-by: Nicolas BACQUEY <[email protected]> Co-authored-by: Nicolas "Niols" Jeannerod <[email protected]>
Co-authored-by: Agustin Mista <[email protected]> Co-authored-by: Alexander Esgen <[email protected]> Co-authored-by: Georgy Lukyanov <[email protected]> Co-authored-by: Thomas BAGREL <[email protected]> Co-authored-by: Nicolas BACQUEY <[email protected]> Co-authored-by: Nicolas "Niols" Jeannerod <[email protected]>
…fusion Co-authored-by: Agustin Mista <[email protected]> Co-authored-by: Alexander Esgen <[email protected]> Co-authored-by: Georgy Lukyanov <[email protected]> Co-authored-by: Thomas BAGREL <[email protected]> Co-authored-by: Nicolas BACQUEY <[email protected]> Co-authored-by: Nicolas "Niols" Jeannerod <[email protected]>
Co-authored-by: Agustin Mista <[email protected]> Co-authored-by: Alexander Esgen <[email protected]> Co-authored-by: Georgy Lukyanov <[email protected]> Co-authored-by: Thomas BAGREL <[email protected]> Co-authored-by: Nicolas BACQUEY <[email protected]> Co-authored-by: Nicolas "Niols" Jeannerod <[email protected]>
Co-authored-by: Agustin Mista <[email protected]> Co-authored-by: Alexander Esgen <[email protected]> Co-authored-by: Georgy Lukyanov <[email protected]> Co-authored-by: Thomas BAGREL <[email protected]> Co-authored-by: Nicolas BACQUEY <[email protected]> Co-authored-by: Nicolas "Niols" Jeannerod <[email protected]>
Co-authored-by: Agustin Mista <[email protected]> Co-authored-by: Alexander Esgen <[email protected]> Co-authored-by: Georgy Lukyanov <[email protected]> Co-authored-by: Thomas BAGREL <[email protected]> Co-authored-by: Nicolas BACQUEY <[email protected]> Co-authored-by: Nicolas "Niols" Jeannerod <[email protected]>
Co-authored-by: Agustin Mista <[email protected]> Co-authored-by: Alexander Esgen <[email protected]> Co-authored-by: Georgy Lukyanov <[email protected]> Co-authored-by: Thomas BAGREL <[email protected]> Co-authored-by: Nicolas BACQUEY <[email protected]> Co-authored-by: Nicolas "Niols" Jeannerod <[email protected]>
785b903 to
4188b8f
Compare
|
I just squashed the fixup commits, and rebased on top of |
…ObjectDiffusion) (#1679) This pull request introduces support for the ObjectDiffusion mini-protocol, required for Peras (for certificates and votes diffusion). It also plugs the PerasCertDiffusion (instance of ObjectDiffusion) mini-protocol in the networking layer. This PR depends on an updated version of `ouroboros-network`, see IntersectMBO/ouroboros-network#5202 and https://github.com/IntersectMBO/ouroboros-network/tree/peras-staging/pr-5202-v2 --- - Added modules `Ouroboros.Consensus.MiniProtocol.ObjectDiffusion{.Inbound,.Outbound}` with implementations of the ObjectDiffusion protocol (quite similar/inspired from TX-submission, except that client = inbound, server = outbound) - Added module `Ouroboros.Consensus.MiniProtocol.ObjectDiffusion.ObjectPool.API` defining `ObjectPool{Reader,Writer}` interfaces, through which ObjectDiffusion accesses/stores the objects to send/that have been received. - Added modules `Ouroboros.Consensus.MiniProtocol.ObjectDiffusion.PerasCert` and `Ouroboros.Consensus.MiniProtocol.ObjectDiffusion.ObjectPool.PerasCert` containing definitions specific to `PerasCert` diffusion through the ObjectDiffusion mini-protocol - Modifies `Ouroboros.Consensus.Node.Serialisation` to add CBOR serialisation (`SerialiseNodeToNode`) for `Point blk`, `Tip blk`, and `PerasCert blk` - Modifies `Ouroboros.Consensus{.Node,.Node.Tracer,.Network.NodeToNode}` to wire-in PerasCertDiffusion similarly to other mini-protocols (e.g. TX-submission) --- - Added module `Test.Consensus.MiniProtocol.ObjectDiffusion.Smoke` with smoke test for the general ObjectDiffusion mini-protocol (using mock objects) - Added module `Test.Consensus.MiniProtocol.ObjectDiffusion.PerasCert.Smoke` with smoke test specific to `PerasCert` diffusion through ObjectDiffusion - Updates `Test.ThreadNet.Network` in `unstable-diffusion-testlib` accordingly to the changes made in `Ouroboros.Consensus.Network.NodeToNode` --- **Network Protocol Version Updates:** - Depends on `ouroboros-network` rev https://github.com/IntersectMBO/ouroboros-network/tree/peras-staging/pr-5202-v2 through source-repository-package directive - Added support in `ouroboros-consensus` for `NodeToNodeV_16`
This pull request introduces support for the ObjectDiffusion mini-protocol, required for Peras (for certificates and votes diffusion). It also plugs the PerasCertDiffusion (instance of ObjectDiffusion) mini-protocol in the networking layer.
This PR depends on an updated version of
ouroboros-network, see IntersectMBO/ouroboros-network#5202 and https://github.com/IntersectMBO/ouroboros-network/tree/peras-staging/pr-5202-v2Peras ObjectDiffusion Mini-protocol:
Ouroboros.Consensus.MiniProtocol.ObjectDiffusion{.Inbound,.Outbound}with implementations of the ObjectDiffusion protocol (quite similar/inspired from TX-submission, except that client = inbound, server = outbound)Ouroboros.Consensus.MiniProtocol.ObjectDiffusion.ObjectPool.APIdefiningObjectPool{Reader,Writer}interfaces, through which ObjectDiffusion accesses/stores the objects to send/that have been received.Ouroboros.Consensus.MiniProtocol.ObjectDiffusion.PerasCertandOuroboros.Consensus.MiniProtocol.ObjectDiffusion.ObjectPool.PerasCertcontaining definitions specific toPerasCertdiffusion through the ObjectDiffusion mini-protocolOuroboros.Consensus.Node.Serialisationto add CBOR serialisation (SerialiseNodeToNode) forPoint blk,Tip blk, andPerasCert blkOuroboros.Consensus{.Node,.Node.Tracer,.Network.NodeToNode}to wire-in PerasCertDiffusion similarly to other mini-protocols (e.g. TX-submission)Tests and benchmarks
Added module
Test.Consensus.MiniProtocol.ObjectDiffusion.Smokewith smoke test for the general ObjectDiffusion mini-protocol (using mock objects)Added module
Test.Consensus.MiniProtocol.ObjectDiffusion.PerasCert.Smokewith smoke test specific toPerasCertdiffusion through ObjectDiffusionUpdates
Test.ThreadNet.Networkinunstable-diffusion-testlibaccordingly to the changes made inOuroboros.Consensus.Network.NodeToNodeNetwork Protocol Version Updates:
ouroboros-networkrev https://github.com/IntersectMBO/ouroboros-network/tree/peras-staging/pr-5202-v2 through source-repository-package directiveouroboros-consensusforNodeToNodeV_16